Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix updating the session history to activate new Documents #6199

Merged
merged 9 commits into from
Dec 11, 2020
Merged

Fix updating the session history to activate new Documents #6199

merged 9 commits into from
Dec 11, 2020
+72 −35

Conversation

domfarolino
Copy link
Member

@domfarolino domfarolino commented Dec 6, 2020 •
edited by pr-preview bot
Loading

Before this PR, the spec made new documents that landed in the #update-the-session-history-with-the-new-page algorithm, and if one of the first two condition branches of Step 3 were matched, these documents were never activated in #traverse-the-history.

After this PR, all new documents created that eventually land in the #update-the-session-history-with-the-new-page algorithm will actually be activated if necessary (set as the browsing context's active document) via #traverse-the-history.

This PR fixes #6197 by doing the following:

(Old proposal) - [x] ~Changing the signature of #update-the-session-history-with-the-new-page to explicitly take a new Document~ - [x] ~Plumb the new document to all call sites of this algorithm~ - [x] ~Change the first conditional branch of #update-the-session-history-with-the-new-page > step 3 to create a new session history entry, explicitly put it in place of the **current entry**, and traverse the history to it~ - [x] ~Update the `"entry update"` history handling behavior dfn to more accurately reflect the current spec text~ - [x] ~Add a note explaining why we have both `"reload"` and `"replace"`, since they are very similar~ - ~If people think this is overkill let me know and I can nix it~ - [x] ~Change the second conditional branch of #update-the-session-history-with-the-new-page > step 3 to explicitly create a new entry that is put in place of the **current entry** in the session history, and traverse to _it_ instead of **current entry**~
  • Modify the #traverse-the-history algorithm to accept a sameEntryTraversal bool
  • Modify #traverse-the-history > Step 5 condition from
    • if (newDocument != current_entry.document), to...
    • if (newDocument != current_entry.document || traverse-to-same-entry)
    • This lets us make changes to current entry but still trigger #traverse-the-history to this existing entry and actually activate its (new) Document
  • Modify #update-the-session[...] > Step 3's first condition to handle entry update and reload in a way that preserves current entry but passes the sameEntryTraversal bool
  • Modify the same-URL condition to compare the correct URLs (fixing the easy problem in Updating the session history: same-URL case is broken #6202)
    • Also uses the replace history handling mode for #traverse-the-history (see my review for justification and mention of an alternative)
  • Add a new condition to #update-the-session[...] > Step 3 that explicitly handles replace by creating a new entry and inserting it after current entry. This certainly seemed to be the spec's intention, but was never properly handled
  • Modify the final path of #update-the-session[...] > Step 3 to assert that the history handling mode is default, since everything else should be handled

The optional task from #6197 can be done as a follow-up:

  • Optional: Decouple the two concepts (a) browsing context's active document, and (b) Window's associated Document`, so that we can set a Window's associated Document ASAP, without setting the browsing context's active document.

/browsing-the-web.html ( diff )

@domfarolino domfarolino requested review from domenic and annevk December 6, 2020 16:05
@domfarolino domfarolino mentioned this pull request Dec 6, 2020
Closed
6 tasks
domfarolino
domfarolino commented Dec 11, 2020
View reviewed changes
Copy link
Member Author

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready for review. The one thing I don't like so much in this PR is that we handle document-replacement and whole-session-entry replacement differently. I feel like we could maybe simplify this in the future by just always replacing the entire entry and conditionally copying over the right state from the old entry depending on the handling mode. This is far from specific to this PR though as we've always had this quirk, so I think it's fine as-is here.

source Show resolved Hide resolved
source Show resolved Hide resolved
domenic
domenic reviewed Dec 11, 2020
View reviewed changes
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@domfarolino
Copy link
Member Author

OK, the latest commit uses the history handling entry update || reload values instead of the sameEntryTraversal bool which was one of the options discussed in your comment about having two related inputs to #traverse-the-history.

This allows us to do as little manual curating of what we pass into #traverse-the-history as possible while still keeping everything simple and correct. Users of #traverse-the-history just need to know which historyHandling value to pass in if they require something other than the default behavior (which is nothing), and they need to know what actions they should take when using their historyHandling value (that is they should know that replace removes the "previous entry" and therefore they should be inserting a new entry after current entry to get it to work right, just like existing call sites).

Let me know if this works for you, otherwise I'm happy to discuss it more and maybe change directions. I'm just slightly partial to not introducing a new slightly-related concept that may require some "translation" for callers of #traverse-the-history.

@domfarolino domfarolino requested a review from domenic December 11, 2020 20:25
@domenic
Copy link
Member

domenic commented Dec 11, 2020

Proposed commit message:

Fix historical and recent breakage in session history traversal

This fixes several related issues surrounding the "update the session
history with the new page" and "traverse the history" algorithms.

* A mistaken attempted refactoring in #6039 got the wrong Document for
  the newDocument variable in "traverse the history". To fix this, we
  explicitly pass in the new document to the algorithm from all its call
  sites. (Discussed in #6197.)

* As discussed more extensively in #6197, the same-URL and entry
  update/reload conditions in "update the session history with the new
  page" were not correctly triggering the new-document clause in
  "traverse the history", despite replacing the document. This was
  partially due to #6039, although the phrasing before #6039 was
  extremely ambiguous, so #6039 was mostly a transition from "unclear"
  to "clearly wrong".

* This fixes the "easy bug" discussed in #6202, where the same-URL case
  was using the wrong URL to determine sameness. That bug was also
  introduced in #6039. The harder bug, as well as the
  action-at-a-distance nature of the same-URL check, remain tracked in
  #6202.

* For years, the spec has required deleting the future session history
  entries after performing a navigation with replacement. This is not
  correct; a with-replacement navigation does not delete future
  navigation entries.

* The latter point makes the handling of same-URL navigations almost
  identical to "replace" navigations (including non-same-URL "replace"
  navigations). This change makes this clear, by using the same text for
  both, but adding a pointer to #6213 which highlights the fact that
  some browsers treat them slightly differently (and some treat them the
  same).

* Finally, this adds or modifies a few assertions to check that we're in
  the "default" history handling behavior, so it's clearer to the reader
  what the non-exceptional path through the spec is.

Closes #6197.

@domfarolino
Copy link
Member Author

LGTM! My only nit would be potentially elaborating on the paragraph:

  • For years, the spec has required deleting the future session history
    entries after performing a navigation with replacement. This is not
    correct; a with-replacement navigation does not delete future
    navigation entries.

... to include a little about what replacement actually does, but that's super picky and possibly already obvious, so I'm fine with whatever.

@domenic domenic merged commit b80e83b into master Dec 11, 2020
@domenic domenic deleted the domfarolino/fix-updating-the-session-history branch December 11, 2020 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domenic domenic domenic approved these changes

@annevk annevk Awaiting requested review from annevk

Assignees
No one assigned
Labels
topic: history
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Updating the session history seems pretty broken
2 participants
@domfarolino @domenic